Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[breaking] do a single bundling with Vite removing esbuild #2580

Closed
wants to merge 2 commits into from

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Oct 10, 2021

Motivation

The second bundling step is a pain. It leads to a whole class of new bugs where users are now exposed to both all Vite bugs and all esbuild bugs (or sometimes not bugs, but purposely omitted features such as lack of TypeScript reflection). It also makes it substantially more difficult to understand difficult edge cases because you need to learn two build systems.

Description

Removes esbuild from the adapter-node and netlify adapters. Rich suggested that Vercel and Cloudflare require everything to be in a single file, so I've left the esbuild step for those adapters

Related issues

Breaking changes

Adapter authors

  • Import app via $server-build alias instead of hard-coded relative path
  • Need to specify serverEntryPoint
  • Simply import @sveltejs/kit/install-fetch directly instead of more complicated shim

Adapter users

  • I removed the options for setting environment variable names in adapter-node. Users can create a custom entry point if they'd like to create environment variables with their own naming scheme
  • Users of adapter-node's entryPoint option should switch to the new serverEntryPoint option

Open questions

  • Do we like the name $server-build for the alias name?
  • Is there some way we can allow typing the app for adapter authors? $server-build doesn't seem to be pulling in types at the moment. I'll leave this for another PR since it's not a regression, but it would be nice to think about

@changeset-bot
Copy link

changeset-bot bot commented Oct 10, 2021

🦋 Changeset detected

Latest commit: d6fc153

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@sveltejs/adapter-cloudflare Patch
@sveltejs/adapter-cloudflare-workers Patch
@sveltejs/adapter-netlify Patch
@sveltejs/adapter-node Patch
@sveltejs/adapter-vercel Patch
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@benmccann benmccann closed this Oct 11, 2021
@benmccann benmccann reopened this Oct 13, 2021
@benmccann benmccann force-pushed the vite-adapter branch 3 times, most recently from a60f3a0 to feec1c8 Compare October 13, 2021 02:56
@benmccann benmccann changed the title Use Vite to do all bundling in a single step removing esbuild step [breaking] replace esbuild bundling of adapters with Vite bundling Oct 13, 2021
@benmccann benmccann force-pushed the vite-adapter branch 2 times, most recently from fff6889 to 55f8652 Compare October 13, 2021 18:53
@benmccann benmccann changed the title [breaking] replace esbuild bundling of adapters with Vite bundling [breaking] do a single bundling with Vite removing esbuild Oct 13, 2021
@benmccann benmccann force-pushed the vite-adapter branch 4 times, most recently from d77931b to 19414ce Compare October 15, 2021 04:16
@benmccann benmccann force-pushed the vite-adapter branch 3 times, most recently from f49099c to 45f7553 Compare October 25, 2021 04:44
@benmccann benmccann force-pushed the vite-adapter branch 6 times, most recently from 68c21fe to 1126f9b Compare October 27, 2021 16:52
@benmccann benmccann added the adapters - general Support for functionality general to all adapters label Oct 27, 2021
@benmccann benmccann marked this pull request as ready for review October 27, 2021 17:13
@benmccann benmccann force-pushed the vite-adapter branch 6 times, most recently from aa76b99 to b579887 Compare November 9, 2021 19:23
@benmccann benmccann force-pushed the vite-adapter branch 3 times, most recently from 176b51a to 373cc10 Compare November 18, 2021 03:57
@vercel
Copy link

vercel bot commented Nov 24, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/svelte/kit/GJZnawTadPuc2Sy8bhuvjzCnEVqP
✅ Preview: https://kit-git-vite-adapter-svelte.vercel.app

@antony
Copy link
Member

antony commented Nov 25, 2021

Just to confirm that I have tested my own app against the netlify adapter here to determine if this fixes the very strange missing transient dependency missing issues that occur when trying to adapt (where some-lib is a transient dependency of an installed package):

> Using @sveltejs/adapter-vercel
 > .svelte-kit/output/server/chunks/__layout-158d2ad5.js:4:7: error: Could not resolve "some-lib"

and can confirm that this PR resolves it.

@tylerbrostrom
Copy link

tylerbrostrom commented Dec 10, 2021

…Cloudflare require[s] everything to be in a single file, so I've left the esbuild step for those adapters.

It should also be possible to remove the esbuild step from adapter-cloudflare and adapter-cloudflare-workers given that JavaScript modules are now supported on Cloudflare Workers (and are the prefered format moving forward).

Granted, this would be a breaking change for adapter-cloudflare-workers, as it uses the older, alternative “service-worker” format.

@Rich-Harris
Copy link
Member

closing in favour of #2931

@Conduitry Conduitry deleted the vite-adapter branch January 11, 2022 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapters - general Support for functionality general to all adapters breaking change
Projects
None yet
5 participants